Skip to content

fix: check gas before perform op #731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

fix: check gas before perform op #731

wants to merge 1 commit into from

Conversation

@einar-polygon
Copy link
Contributor

@einar-polygon einar-polygon commented Oct 17, 2024 •
edited
Loading

This is a quick attempt to fix the bug align the unexpected behavior in block 19794433 tx 0xb487d37c4f13407af55797add295c127f19ea6c823b8ba4e53fe1a3457d902c9 where JUMPs get performed and their destination added to the JUMPDEST table despite of an Out-of-Gas condition.

I am going to add a regression test as part of this PR.

Offending instruction from RPC structlog:

{
 "step": 1198,
 "curr_depth": 2,
 "tx_hash": "0xb487d37c4f13407af55797add295c127f19ea6c823b8ba4e53fe1a3457d902c9",
 "code_hash": "0xb9c1c929064cd21734c102a698e68bf617feefcfa5a9f62407c45401546736bf",
 "ctx": 2,
 "pc": 563,
 "pc_hex": "00000233",
 "gas": 2,
 "gas_cost": 10,
 "op": "JUMPI",
 "entry": "StructLog { pc: 563, op: \"JUMPI\", gas: 2, gas_cost: 10, depth: 2, error: None, stack: Some([599290589, 10000, 46305234306404416513646796018318532483072908513802138792889746402715613072618, 131758842981497199554169577046225233044076042219, 294, 131758842981497199554169577046225233044076042219, 128, 10000, 0, 0, 288, 0, 1, 567]), return_data: None, memory: None, memory_size: None, storage: None, refund_counter: None }"
}

@einar-polygon einar-polygon added the bug Something isn't working label Oct 17, 2024
@einar-polygon einar-polygon self-assigned this Oct 17, 2024
@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Oct 17, 2024
@Nashtare
Copy link
Collaborator

Nashtare commented Oct 17, 2024 •
edited
Loading

Just to clarify, is this "bug" affecting develop too or is it purely related to the structlogs interaction in the JD branch?
I'd tend to believe it's the latter (in which case we may want to target your opened PR instead of develop, but I haven't tested it)

@einar-polygon
Copy link
Contributor Author

I am arguing for the former as follows:

develop does:

perform_op
check_gas

where this PR does:

check_gas
perform_op

The consequence is, that the jump is performed before the OOG is detected. The endpoint does not perform the jump.
This difference results in the extra JD offset in the simulation.

0000022d: DUP3
0000022e: DUP1
0000022f: ISZERO
00000230: PUSH2 0x237
00000233: JUMPI        <---- OOG
00000234: DUP3
00000235: DUP3
00000236: RETURN
00000237: JUMPDEST     <---- shouldn't be in the reached set.

@Nashtare
Copy link
Collaborator

Hmm, the current develop doesn't run into any issues with the failed JUMP though (I just tested it, as OOG exceptions are properly caught), this ordering logic seems specific to the way structlogs report the ops.

As long as the switch is harmless with simulations, it's fine to merge against develop.

@einar-polygon einar-polygon removed the bug Something isn't working label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nashtare Nashtare Awaiting requested review from Nashtare Nashtare will be requested when the pull request is marked ready for review Nashtare is a code owner

@LindaGuiga LindaGuiga Awaiting requested review from LindaGuiga LindaGuiga will be requested when the pull request is marked ready for review LindaGuiga is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@einar-polygon einar-polygon

Labels

crate: evm_arithmetization Anything related to the evm_arithmetization crate.

Projects

Zero EVM
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@einar-polygon @Nashtare